-
Notifications
You must be signed in to change notification settings - Fork 1
[PEP - Wheel Variant] Staging before push to upstream #6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: pep-wheel-variants-acceptance
Are you sure you want to change the base?
[PEP - Wheel Variant] Staging before push to upstream #6
Conversation
Done via: ``` m2r2 --no-underscore-emphasis pepxxx_wheel_variant_support.md ``` Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
The pandoc's conversion is much better. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
| Konstantin Schütze <[email protected]>, | ||
| Ralf Gommers <[email protected]>, | ||
| Andrey Talman <[email protected]>, | ||
| Charlie Marsh <[email protected]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Do you mind using [email protected] (if not [email protected])?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with either, I think I've copied that from your commit messages. Just tell me which one you prefer (or "suggest" a change).
|
|
||
| 1. If ``install-time`` is true, the dictionary describes an install-time | ||
| provider and the ``requires`` key must be present and specify at | ||
| least one dependency. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because that "one dependency" is, presumedly, the Python package containing the provider itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It's basically "empty requires make no sense because then you don't have anything to call".
Co-authored-by: Charlie Marsh <[email protected]>
Co-authored-by: Charlie Marsh <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
Co-authored-by: Emma Smith <[email protected]>
Per #6 (comment). Signed-off-by: Michał Górny <[email protected]>
|
I've already replied to specific comments, but I think these two points are worth discussing top-level:
I think the PEP as it stands right now roughly splits tools into "installers" and "everything else". The boundary between the two is a bit blurry, and I don't think we're really following it consistently. The primary idea behind "installers" is that these are packages that need to be able to determine whether variant wheels are supported, and may be executed with elevated privileges. Therefore, from the security PoV there is a risk that variant wheels would require installing and running arbitrary packages with these privileges. The "everything else" is basically tools where we don't worry about that — e.g. tools that only need to process wheels without determining whether they are supported, or build tools that install and run arbitrary packages anyway. I suppose "installers" is a bit of a misnomer, since there are technically other tools that need to determine whether wheels can be installed. However, I don't think "resolver" is a good replacement. I mean, saying that "resolver should vendor a plugin" is weird — the installer does that, resolver is merely one of its components. Furthermore, the same applies to cases where resolver isn't really used, e.g. when installing a wheel from the local filesystem. I'm open to improving things, but I don't really have a better term than "installer", and I'm not sure if using a strictly more correct term won't make things harder to comprehend.
Indeed. Probably the "security considerations" section needs to be rewritten to make the problem clearer. I wonder if it would make sense to move it prior to specification; describe the problem there, then the measures taken in the specification part. |
Since resolvers are part of other tools like lockfile generators (e.g. pip-tools), I view resolvers as a component shared between installers and lockfile generators. Resolvers are used elsewhere too (e.g. tools to check licenses of various dependencies, etc.).
Yeah I think my main point is that the PEP makes specifications about package resolution, but tools other than installers do that.
One thing we could do is define "resolvers" as "tools which has a package resolution step". Or even use "resolver tools" (but maybe that is verbose).
I agree this seems like an improvement! Generally people don't move the sections around, but I think that should be fine since it is important to the flow of the PEP. We could also have a security section in the rationale I suppose. |
Co-authored-by: Emma Smith <[email protected]>
* Update the bit about SciPy Thanks to @rgommers in #6 (comment). Signed-off-by: Michał Górny <[email protected]> * Sync quotes from wheelnext/wheelnext#110 Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
|
I'll prepare an update for the security-related stuff tomorrow. |
Per #6 (comment). Signed-off-by: Michał Górny <[email protected]>
Don't use tool-specific option, mention the caching problem and better describe why it's no build isolation.
The regular spaces are ASCII characters, while the non-breaking spaces are special Unicode characters. There's no need in our text to use non-breaking spaces, such as there would be with SI units.
Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
#6 (comment) Signed-off-by: Michał Górny <[email protected]>
Co-authored-by: Emma Smith <[email protected]>
konstin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding some notes, filed PRs for some others.
| - A **variant provider plugin** interface that allows installers to dynamically detect | ||
| platform attributes and select the most suitable wheel. | ||
|
|
||
| The goal is to simplify the user experience to a familiar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstract is already long, I'd cut the paragraph below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in cut if after the command, or remove the paragraph entirely?
| ``manylinux_2_34_x86_64`` now implicitly requires ``x86_64-v2`` with no | ||
| support for other x86 version. This lack of support results in | ||
| complexity in managing platform-specific dependencies and compatibility. | ||
| That complexity affects the installation process for users and increases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem here is really that it's impossible to express any or different x86_64 versions, so you have to pick an oldest version you support and ship only that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it hard to understand your point. FWIU the issue there is that packages implicitly require v2 and we can't express that. Being able to ship multiple wheels is tangential to that.
| slightly. The last two bars are labeled "skylake_avx512" and | ||
| "cascadelake" (both "AVX512") and reach almost 2.0 ns/day. | ||
|
|
||
| GROMACS AVX512 Benchmark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the GROMACS benchmark, and what do the numbers refer to? The figure description should contain a concise description of what is being measured, just enough for a Python programmer who hasn't heard GROMACS before to understand the impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| specification. The link to this file must be present on all index pages | ||
| where the variant wheels are linked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We treat the variants json file like an archive file (not like a .metadata file), it's a regular top level enty, including having a hash. I'll file a PR.
Signed-off-by: Michał Górny <[email protected]>
Co-authored-by: Emma Smith <[email protected]>
Not sure if we should do that as part of the PEP submission, but we are using lexers available since 2.19.0, as @konstin noticed. Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
Co-authored-by: konsti <[email protected]>
Signed-off-by: Michał Górny <[email protected]>
* Put more focus on torch * Update peps/pep-0815.rst Co-authored-by: Michał Górny <[email protected]> * Update peps/pep-0815.rst Co-authored-by: Michał Górny <[email protected]> * Update peps/pep-0815.rst Co-authored-by: Michał Górny <[email protected]> --------- Co-authored-by: Michał Górny <[email protected]>
No description provided.